repl: migrate errors to internal/errors by kysnm · Pull Request #17716 · nodejs/node

kysnm · 2017-12-17T11:46:19Z

Refs: #17709

cc @jasnell @joyeecheung

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc, errors, repl

kysnm · 2017-12-17T12:23:50Z

It is failed at test/async-hooks/test-callback-error.js on my local environment, but master branch also fails.

Trott · 2017-12-17T17:45:21Z

@kysnm Sounds like you may be experiencing #15985 on your local machine. If you do any troubleshooting and find information that may be useful, please put it in that issue! (And if you find a fix for the problem, please submit a pull request!)

Otherwise, yes, that is most likely unrelated to the change here.

CI: https://ci.nodejs.org/job/node-test-pull-request/12157/

Trott · 2017-12-17T17:45:57Z

Pinging @nodejs/tsc since this will need two TSC approvals.

joyeecheung · 2017-12-17T19:52:27Z

lib/repl.js


-        if (err && err.message === 'Script execution interrupted.') {
+        if (err && err.message ===
+          errors.message('ERR_SCRIPT_EXECUTION_INTERRUPTED')) {


I think you can check if err.code === 'ERR_SCRIPT_EXECUTION_INTERRUPTED' here instead (which is kinda the point of this PR, the code instead of the message should be used to match a type of error)

joyeecheung · 2017-12-17T19:56:19Z

test/parallel/test-internal-errors.js

 }

+// Test ERR_SCRIPT_EXECUTION_INTERRUPTED
+assert.strictEqual(


This test does not look particularly useful since the error message formatter does not take any parameters for this error.

joyeecheung · 2017-12-17T19:59:12Z

test/parallel/test-repl-top-level-await.js

    'await timeout(100000)\r',
-    'Thrown: Error: Script execution interrupted.',
+    'Thrown: Error [ERR_SCRIPT_EXECUTION_INTERRUPTED]: ' +
+      errors.message('ERR_SCRIPT_EXECUTION_INTERRUPTED'),


Since we are actually testing the error message, we don't really need to generate it with errors.message, can you change this to match the actual error message instead?

joyeecheung · 2017-12-17T20:02:29Z

There are several tests that do incomplete matches against this error message, those should be updated as well, not necessarily in this PR though.

joyeecheung · 2017-12-17T20:08:30Z

doc/api/errors.md

+<a id="ERR_SCRIPT_EXECUTION_INTERRUPTED"></a>
+### ERR_SCRIPT_EXECUTION_INTERRUPTED
+
+A script execution interrupted.


Can you elaborate a bit more on this? For example:

Script execution was interrupted by `SIGINT` (Ctrl+C).

kysnm · 2017-12-18T17:36:20Z

@joyeecheung
cc @jasnell

I have fixed as you pointed out. Please review.

Trott · 2017-12-18T21:33:18Z

Nit, just a suggestion, not a blocking objection: I think we should probably leave it at SIGINT and not add Cntl+C since there are other ways to send SIGINT that don't involve control+c.

kysnm · 2017-12-18T23:25:36Z

@Trott
Thanks for your review. I almost agree with you. But, It is included Ctrl+C in a few docs. How do you think?
(I know. deps is almost irrelevant ...)

deps/v8/tools/testrunner/network/network_execution.py:        # Use a timeout so that signals (Ctrl+C) will be processed.
doc/api/vm.md:    `SIGINT` (Ctrl+C) is received. Existing handlers for the

joyeecheung · 2017-12-19T04:05:49Z

doc/api/errors.md

+<a id="ERR_SCRIPT_EXECUTION_INTERRUPTED"></a>
+### ERR_SCRIPT_EXECUTION_INTERRUPTED
+
+Script execution was interrupted by `SIGINT` (Ctrl+C).


I kind of agree with @Trott that this is not exactly accurate. Maybe here we should say (for example, when Ctrl+C was pressed) instead

joyeecheung · 2017-12-19T04:06:32Z

lib/internal/errors.js

 E('ERR_PARSE_HISTORY_DATA', 'Could not parse history data in %s');
 E('ERR_REQUIRE_ESM', 'Must use import to load ES Module: %s');
+E('ERR_SCRIPT_EXECUTION_INTERRUPTED',
+  'Script execution was interrupted by `SIGINT` (Ctrl+C).');


We probably don't need the part in the parentheses in the error message. The details should be in the docs.

kysnm · 2017-12-19T12:22:58Z

@joyeecheung @Trott
cc @jasnell

Fixed!!

joyeecheung · 2017-12-20T10:19:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12226/

mcollina

LGTM

joyeecheung · 2017-12-22T03:04:21Z

Previous CI was stopped. New CI: https://ci.nodejs.org/job/node-test-pull-request/12257/

maclover7 · 2017-12-22T18:02:10Z

Landing...

maclover7 · 2017-12-22T18:28:19Z

Thank you for your contribution, landed in ab5a2ab.
❤️ 💚 💙 💛 💜

repl: migrate errors to internal/errors

f237327

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. repl Issues and PRs related to the REPL subsystem. labels Dec 17, 2017

Trott added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 17, 2017

joyeecheung reviewed Dec 17, 2017

View reviewed changes

kysnm added 4 commits December 19, 2017 02:18

Check the err.code instead of the err.message

4f918bb

Remove useless test

65fc1e8

Change the contents of the error message in detail

fed6d47

Check the actual error message instead of the generate it

3dcabb7

joyeecheung reviewed Dec 19, 2017

View reviewed changes

kysnm added 2 commits December 19, 2017 21:18

Describe more details at the docs.

480358b

Remove useless parenthses.

f142f84

jasnell approved these changes Dec 19, 2017

View reviewed changes

joyeecheung mentioned this pull request Dec 20, 2017

Tracking issue: Migrate errors to internal/errors.js #17709

Closed

6 tasks

joyeecheung approved these changes Dec 20, 2017

View reviewed changes

mcollina approved these changes Dec 20, 2017

View reviewed changes

maclover7 approved these changes Dec 22, 2017

View reviewed changes

joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 22, 2017

BridgeAR approved these changes Dec 22, 2017

View reviewed changes

maclover7 self-assigned this Dec 22, 2017

maclover7 closed this Dec 22, 2017

joyeecheung removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 23, 2017

Uh oh!

Conversation

kysnm commented Dec 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

kysnm commented Dec 17, 2017

Uh oh!

Trott commented Dec 17, 2017

Uh oh!

Trott commented Dec 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung Dec 17, 2017

Choose a reason for hiding this comment

Uh oh!

joyeecheung Dec 17, 2017

Choose a reason for hiding this comment

Uh oh!

joyeecheung Dec 17, 2017

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Dec 17, 2017

Uh oh!

joyeecheung Dec 17, 2017

Choose a reason for hiding this comment

Uh oh!

kysnm commented Dec 18, 2017

Uh oh!

Trott commented Dec 18, 2017

Uh oh!

kysnm commented Dec 18, 2017

Uh oh!

joyeecheung Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung Dec 19, 2017

Choose a reason for hiding this comment

Uh oh!

kysnm commented Dec 19, 2017

Uh oh!

joyeecheung commented Dec 20, 2017

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Dec 22, 2017

Uh oh!

maclover7 commented Dec 22, 2017

Uh oh!

maclover7 commented Dec 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

kysnm commented Dec 17, 2017 •

edited

Loading

Trott commented Dec 17, 2017 •

edited

Loading

joyeecheung Dec 19, 2017 •

edited

Loading